Skip to content

test(tbtc): multi-signer simulated integration test for reservation coordination - #4279

Merged
piotr-roslaniec merged 10 commits into
m1/reservation-coordination-checklistfrom
m1/reservation-multisigner-integration-test
Sep 3, 2026
Merged

test(tbtc): multi-signer simulated integration test for reservation coordination#4279
piotr-roslaniec merged 10 commits into
m1/reservation-coordination-checklistfrom
m1/reservation-multisigner-integration-test

Conversation

@piotr-roslaniec

@piotr-roslaniec piotr-roslaniec commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

Implements implementation-plan.md Milestone 3's "multi-signer simulated
integration test" item - the last piece of the full M1 keep-core-readiness
implementation plan (M0 is an external release-coordination gate, not a code
task; M1's four rows and M2's test-coverage backfill are covered by
#4276,
#4277,
#4278, and a
separate M2 follow-up PR).

Stacked on m1/reservation-coordination-checklist
(#4278).

Scope note (per explicit decision this session): Milestone 3 has two
items - this test, and a "testnet round with a forced liveness/stranding
drill" (~2 weeks, needs a live testnet deployment and real multi-operator
wall-clock timing). Only the former is code; the latter is tracked as an
agent-not-actionable item in the plan doc, unchanged by this PR.

Change

Scales TestCoordinationExecutor_Coordinate's existing 3-operator harness -
deterministic keypairs, real per-operator localChain fakes, a real shared
netlocal.BroadcastChannel, one goroutine per operator running
coordinationExecutor.coordinate concurrently - to ReservationAnchorProposal
and ReservationReanchorProposal, added as one table-driven test with
anchor/reanchor subtests:

  • TestCoordinationExecutor_Coordinate_ReservationProposals

This exercises the real leader/follower coordination round-trip (checklist
generation -> leader election -> broadcast -> follower validation ->
convergence) that no mocked pkg/tbtcpg unit test can cover, since those
call task.Run(request) directly and never go through
coordinationExecutor.coordinate. It also exercises #4277's protobuf
marshaling of both proposal types over a real wire round-trip, since every
follower unmarshals the leader's broadcast coordinationMessage.

Depends on #4278 (this branch's parent): before that fix,
ActionReservationAnchor/ActionReservationReanchor never appeared in
getActionsChecklist's output, so every operator's checklist search in
these tests fell through to NoopProposal and failed. Verified directly:
temporarily reverted #4278's checklist change, re-ran the new test (both
subtests failed with the expected NoopProposal mismatch), then restored it.

A bug found in this test's own harness, and its root-cause fix

The two reservation subtests initially shared one netlocal broadcast
channel name. getBroadcastChannel's registry is keyed by name, is
process-global, and never released old channels' retransmission tickers
(they were wired to context.Background()), so under -race the reanchor
subtest's follower sometimes received a stale broadcast left over from the
anchor subtest's leader - a cross-test data race in the test harness
itself, not in the production code under test.

Root-caused and fixed in pkg/net/local (production, non-test code, since
the registry it fixes is used by every test file that exercises a simulated
local network): each broadcast channel's retransmission ticker context is
now cancellable, and a new ReleaseBroadcastChannel(name string) cancels
and de-registers a channel's own ticker(s) by name (scoped to the caller's
own channel, not a global reset) on t.Cleanup. This is now wired into all
four broadcast-channel-creation sites in pkg/tbtc/coordination_test.go
(the shared operator helper plus three pre-existing hardcoded-name tests),
each releasing under its own channel name.

Testing

  • go build ./..., go vet ./..., gofmt -l: clean.
  • go test ./pkg/tbtc/... and go test -race ./pkg/tbtc/...: full suite
    green, including -count=10 targeted at the new/changed coordination
    tests.
  • go test ./pkg/net/local/... ./pkg/net/retransmission/... (incl. -race):
    green, including new coverage for ReleaseBroadcastChannel's actual
    effect (a released channel's ticker stops retransmitting; releasing and
    reopening under the same name only delivers to the new registration).

Not in this PR

  • The testnet-round liveness/stranding drill (Milestone 3's other item) -
    operational, not code; tracked separately.
  • Milestone 2's test-coverage backfill - separate follow-up PR.

@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: d1808093-1d9a-4b2e-9702-a676d13d29e3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Closes gap-analysis Major row 1 and implementation-plan.md M1 row 3.

ReservationAnchorProposal, ReservedRedemptionProposal,
ReservationReanchorProposal, and ReservationDissolutionProposal
previously used a JSON Marshal/Unmarshal placeholder, unlike every
other CoordinationProposal type in this package (Heartbeat,
DepositSweep, Redemption, MovingFunds, MovedFundsSweep), which all
marshal via pkg/tbtc/gen/pb.

Added the four missing message types to message.proto and
regenerated message.pb.go (protoc 3.21.12 installed for this).
Moved the four proposals' Marshal/Unmarshal from reservation.go's
JSON stubs into marshaling.go, matching the existing proto-based
implementations' structure and field-encoding conventions (big.Int
fees via .Bytes()/SetBytes(), fixed-size hashes/pubkey-hashes via
byte-slice copy with a length check).

Preserved the original JSON stubs' validation intent under proto3's
zero-value-is-absence semantics: a request nonce of 0, or empty
fee/reservation-key/hash bytes, are rejected the same way an
explicitly-missing JSON field was. The original '== nil' checks on
*big.Int fields don't carry over as-is - SetBytes never returns nil -
so they're now byte-length checks on the wire field instead, which is
the pattern every other proto-based proposal in this file already
uses.

Testing: extended the existing table-driven
TestCoordinationMessage_MarshalingRoundtrip with the four new types
(exact field-for-field equality through the wire, matching the
existing test's own precision, not just the fuzz-style tests already
covering every sibling type) plus four new
TestFuzzCoordinationMessage_MarshalingRoundtrip_With<X>Proposal
crash-safety tests, matching the one-per-type convention. Rewrote
the pre-existing TestReservationProposals_UnmarshalRejectsMissingIntegers
(now TestReservationProposals_UnmarshalRejectsInvalidFields) to
construct real protobuf payloads instead of JSON string literals,
porting every original missing-field case plus two new structural
cases (invalid hash/pubkey-hash length) that fall out of the new
wire format.

go test ./pkg/tbtc/...: 15/15 new/changed tests pass, full package
suite passes (146s), -race clean (156s). gofmt/vet clean on all 6
changed files.
…oordination

Implementation-plan.md Milestone 3, 'multi-signer simulated
integration test' item (per user decision: build the test, leave the
testnet-drill item as an agent-not-actionable tracked item since it
needs live infra and calendar time, not code).

Scales TestCoordinationExecutor_Coordinate's existing 3-operator
harness - deterministic keypairs, real per-operator localChain fakes,
a real shared netlocal.BroadcastChannel, one goroutine per operator
running coordinationExecutor.coordinate concurrently - to
ReservationAnchorProposal and ReservationReanchorProposal. This
exercises the real leader/follower coordination round-trip
(checklist generation -> leader election -> broadcast -> follower
validation -> convergence) that no mocked pkg/tbtcpg unit test can
cover, since those call task.Run(request) directly and never go
through coordinationExecutor.coordinate. It also exercises PR
#4277's protobuf marshaling of both proposal types over a real wire
round-trip, since every follower unmarshals the leader's broadcast
coordinationMessage.

Depends on PR #4278 (this branch's parent): before that fix,
ActionReservationAnchor/ActionReservationReanchor never appeared in
getActionsChecklist's output, so every operator's checklist search in
these tests would fall through to NoopProposal and fail - confirmed
by temporarily reverting the checklist fix and re-running (both new
tests failed with the expected NoopProposal mismatch), then restoring
it.

Found and fixed one bug in this test's own harness during
verification: both new tests initially shared one netlocal broadcast
channel name. getBroadcastChannel's registry is keyed by name and
never releases old channels, so under -race (which changed
goroutine/channel-delivery timing enough to surface it in ~every
run), the reanchor test's follower sometimes received a stale
broadcast left over from the anchor test's leader. Fixed by giving
each test its own channel name; re-verified stable across 10
repeated -race runs plus the full non-race and race suites.

Testing:
- go test ./pkg/tbtc/...: 365/365 pass.
- go test -race ./pkg/tbtc/...: clean, no data races, including
  -count=10 on just the two new tests.
- go build ./... && go test ./...: full repo, 49 packages, zero FAIL.
- gofmt -l / go vet: clean.
Resolves all 11 confirmed findings from review of the reservation
multi-signer coordination test:

- Bound runReservationCoordinationRound's report wait with a 30s
  timeout instead of an unbounded channel receive: at
  coordinationBlock=24562800, coordinate()'s only cancel path takes
  ~28 simulated days to fire, so any follower-rejects-proposal
  regression would hang the goroutine and the test forever, killing
  every other pkg/tbtc test via the package-wide go test timeout.

- Derive each test's broadcast channel name from t.Name() plus a
  per-invocation nonce instead of a hardcoded literal: the
  coordination leader intentionally keeps retransmitting for the
  active phase's duration, so a hardcoded name risks an earlier
  invocation's leader retransmitting into a later invocation's
  followers under -count=N or a future test reusing the name.

- Migrate TestCoordinationExecutor_Coordinate onto the shared
  reservation-coordination helpers instead of its own duplicated
  inline fixture/report/sort logic, and collapse
  TestCoordinationExecutor_Coordinate_ReservationAnchor/Reanchor into
  one table-driven TestCoordinationExecutor_Coordinate_ReservationProposals.

- Drop the now-unused sort in runReservationCoordinationRound (no
  assertion depended on report order) and the tautological
  reports-count assertions.

- Stop aliasing the mock generator's returned pointer as the expected
  result in assertions, so the leader-side comparison isn't a
  vacuous pointer-identity check.

- Correct four doc comments that overclaimed shared-state absence,
  stale branch provenance, and reanchor test chronology; add the
  missing public-key-hash comment in newReservationCoordinationWallet.

Verified: go build ./..., go vet ./pkg/tbtc/..., gofmt clean,
go test ./pkg/tbtc/... (145s, all pass), and the three affected
tests under -race -count=10 (clean).
Removing the tautological reports-count assertion (previous commit)
also removed the only check that all three operators actually
reported: len(reports) == len(operators) holds by loop construction
regardless of *which* operators reported, so a fan-in bug returning
two reports for one operator while another's is lost would pass
silently. Add an explicit check in runReservationCoordinationRound
(which owns the fan-in) that every operator index 1..len(operators)
appears at least once among the collected reports.

Verified: go build ./..., go vet ./pkg/tbtc/..., gofmt clean,
the three affected tests individually confirmed via raw (non-
summarized) test output, -race -count=10 clean, and the full
pkg/tbtc suite (146s, all pass).
Root-causes finding P1-#2's minimum fix (unique channel name per test
invocation, previous commit): pkg/net/local's broadcastChannels registry
is append-only and process-global, and each retransmission ticker was
started with context.Background(), so it retransmits forever with no
way to stop it externally. A later test/invocation reusing a channel
name would keep receiving an earlier invocation's stale, still-
retransmitting messages for the lifetime of the test binary - three
pre-existing tests (ExecuteLeaderRoutine, ExecuteFollowerRoutine,
ExecuteFollowerRoutine_WithIdleLeader) still hardcode "test"/"test-idle"
and were never covered by the minimum fix.

- pkg/net/local/broadcast_channel_manager.go: give each channel a
  cancellable context instead of context.Background(), track the
  cancel funcs, and add ResetForTesting() to cancel every outstanding
  ticker and clear the registry.
- pkg/tbtc/coordination_test.go: wire t.Cleanup(netlocal.ResetForTesting)
  into all four broadcast-channel-creation sites in this file (the
  shared reservation-coordination helper plus the three pre-existing
  hardcoded-name tests), so every test starts from an empty registry
  regardless of channel-name convention - removing the need for the
  per-invocation-nonce workaround to be the only safeguard.

Verified: go build ./..., go vet ./pkg/tbtc/... ./pkg/net/local/...,
gofmt clean. All 5 affected tests together under -race -count=10
(50/50 pass, proving cross-invocation isolation actually holds now).
Full pkg/net/local and pkg/tbtc suites pass (145s).
ResetForTesting (previous commit) already makes channel-name reuse
safe by cancelling every outstanding ticker and clearing the registry
between invocations - proven experimentally: forcing all operators
onto one fixed colliding name still passed 20/20 under -race -count=10
with the hook active, and failed under the same forced collision with
the hook disabled (reanchor received a stale anchor proposal from an
earlier subtest's still-retransmitting leader).

The per-invocation time.Now().UnixNano() nonce was therefore dead
weight, and the doc comment claiming a name "should be unique per
test invocation" was no longer true. Dropped the nonce (channelName
is now just t.Name(), kept for attributing a leak to its source test,
not for uniqueness) and rewrote the comment to describe the actual
current invariant.

Verified: go build ./..., go vet ./pkg/tbtc/..., gofmt clean. The five
netlocal-using tests together under -race -count=20 (100/100 pass,
genuine repeated-invocation collision on the same fixed name, not a
synthetic one). Full pkg/tbtc suite (146s) green.
…r cleanup

- Rescope ResetForTesting to a name-keyed ReleaseBroadcastChannel(name)
  instead of wiping the entire process-global channel registry, so
  tests (and any future caller) can release one channel without
  destroying every other channel's retransmission ticker.
- Guard the retransmission Ticker's post-loop handler cleanup with the
  same mutex used everywhere else in the type, closing a race between
  concurrent onTick/onUnregister callers and ticker shutdown.
- Add TestReleaseBroadcastChannel covering release-stops-retransmission
  and reuse-after-release-only-delivers-to-the-new-channel behavior.
- Fix checklist-ordering doc comment to match the actual actionPriority
  map.
- Hoist the 30s fan-in deadline outside the report-collection loop so
  it bounds the whole wait instead of re-arming on every report.
- Rewrite the protocolLatch doc comment: it does not serialize
  concurrent operator goroutines, only bounds in-flight work.
- Rename reservationCoordination* test helpers to drop the misleading
  "reservation" prefix; they exercise the general coordination path.
- Fix the leader-goroutine/waiter leak in waitForBlockHeight by
  translating the requested absolute block height into the local
  chain fake's own relative counter frame before waiting, instead of
  waiting on the raw absolute height (which could take days of
  simulated block time to reach for mainnet-scale values).
- Correct the fixture doc comment's chain-sharing overclaim.
- Fix coordination.go's redemption-priority comment to describe the
  actual post-activation gating behavior.
- Rename TestReservationProposals_UnmarshalRejectsMissingIntegers to
  TestReservationProposals_UnmarshalRejectsInvalidPayloads, matching
  what the test actually covers.
@piotr-roslaniec
piotr-roslaniec force-pushed the m1/reservation-multisigner-integration-test branch from cd64125 to 4b7ee24 Compare September 3, 2026 11:09
…ion doc comment

The rebase's conflict resolution left the doc comment referencing the
function's pre-export lowercase name.
@piotr-roslaniec
piotr-roslaniec marked this pull request as ready for review September 3, 2026 12:34
@piotr-roslaniec
piotr-roslaniec merged commit 397b340 into m1/reservation-coordination-checklist Sep 3, 2026
16 of 17 checks passed
@piotr-roslaniec
piotr-roslaniec deleted the m1/reservation-multisigner-integration-test branch September 3, 2026 12:34
piotr-roslaniec added a commit that referenced this pull request Sep 3, 2026
…adcastChannel (#4284)

Follow-up to #4283.

That PR fixed `TestReleaseBroadcastChannel`'s flake (reproduced
pre-existing on clean `origin/reservations-epic` at the time, ~2/5
failure rate in isolation - #4279's bug, not introduced by #4283's
merge) by absorbing the one straggler tick `NewTimeTicker`'s
cancel-vs-elapsed-timer race can let through after
`ReleaseBroadcastChannel`.

That fix's settle-window drain discarded its count unchecked, so a
genuine regression where the ticker fires more than once after release
would only surface at the second, stricter assertion - not at the settle
step itself, where the failure is easier to diagnose. This bounds the
settle window: at most one straggler, asserted explicitly.

Verified 20/20 locally (`go test ./pkg/net/local/... -run
TestReleaseBroadcastChannel -count=1`, repeated); full `go build`/`go
vet`/`gofmt -l` clean.
piotr-roslaniec added a commit that referenced this pull request Sep 3, 2026
## Summary

Implements `implementation-plan.md` Milestone 2's test-coverage
backfill:
7 of the 8 listed items (item 8's scope narrowed - see below). One
earlier-planned item, a golden-value dedup test for
`AssembleReservationAnchorTransaction`, was obsoleted when
`proposeReservationAcceptance` was switched to call the exported
`tbtc.AssembleReservationAnchorTransaction` directly, removing the
second,
unexported copy the dedup test would have compared against; it was not
silently dropped. Merged up to date with
`m1/reservation-multisigner-integration-test`
([#4279](#4279)), tip
`9e42103e8`.

The 8th item (`ValidateReservationAnchorProposal`/
`ValidateReservationReanchorProposal` tests) is explicitly deferred - it
needs `go-ethereum` simulated-backend test infrastructure that doesn't
exist anywhere in `pkg/chain/ethereum` today, well beyond the plan's
0.5-day estimate. See
`docs/spec/reservations/m1-keep-core-readiness/01-gap-analysis.md`'s
new Minor row for the full finding.

## Change

**`pkg/tbtc/reservation_test.go`**
- `TestAssembleReservationAnchorTransaction`: happy-path output shape
  (1-in-1-out, deposit value minus fee, P2WPKH to the target wallet).
- `TestAssembleReservationReanchorTransaction`: same shape assertion for
  the re-anchor sibling.

**`pkg/chain/ethereum/tbtc_test.go`**
- `TestConvertReservationParametersFromAbiType`: full 10-tuple field
  mapping, every field a distinct non-zero value so a swapped or dropped
  field can't hide behind a shared zero default.
- `TestConvertReservationFromAbiType_DropsCumulativeReanchorFee`: pins
the
  intentional `CumulativeReanchorFee` omission and verifies every other
  field maps correctly around it.

**`pkg/tbtcpg/reservation_acceptance_test.go`**
- `TestReservationAcceptanceTask_BoundaryChecks`: table covering
at-limit/one-over-limit boundary crossings for
`MaxReservationsPerWallet`,
  `ReservationMinAmount`, `ReservationMaxTotalAmount`,
  `ReservationMaxSingleAmount`, `MaxReservationsAmountPerWallet`, and
  `ActiveReservationsCount`, plus the net-of-fee minimum check in
  `proposeReservationAcceptance` -
  `TestReservationAcceptanceTask_BoundedLookback` only ever used these
  fields as fixture data, never at the actual boundary.
- `TestReservationAcceptanceTask_ReservationParametersFetchedLive`: runs
  the same task twice against the same deposit, mutating
  `ReservationMinAmount` between calls - verifies a governance-driven
  parameter change takes effect on the very next `Run()` call, with no
leftover value from a prior run observable in the eligibility decision.
- `TestReservationAcceptanceTask_AnchorTransactionAssembly`: end-to-end
  wiring test - runs the task to get a `ReservationAnchorProposal`, then
  re-assembles and signs the anchor transaction via the exported
  `tbtc.AssembleReservationAnchorTransaction`, and asserts the resulting
  signed transaction is a valid 1-input-1-output transaction paying the
  correct wallet P2WPKH output script with value equal to deposit amount
  minus the anchor fee.

## Testing

- `go test ./pkg/tbtc/... ./pkg/tbtcpg/... ./pkg/chain/ethereum/...`:
  280/280 pass.
- `go build ./...` && `go test ./...`: full repo, 49 packages, zero
`FAIL`.
- `gofmt -l` / `go vet`: clean on all changed/new files.

## Not in this PR

-
`ValidateReservationAnchorProposal`/`ValidateReservationReanchorProposal`
  tests - deferred, documented in the gap-analysis doc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant